v1.3.1: Minor fixes for Firefox & Edge #56
Conversation
Bumps [nanoid](https://github.com/ai/nanoid) from 3.3.7 to 3.3.8. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.3.7...3.3.8) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Adapt firefox * support firefore auto publish * support firefox, and optimized UX
* [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * update English i18n msg --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: SYM01 <33443792+SYM01@users.noreply.github.com>
* [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
* [WIP] support chrome auto deploy * finalize Chrome auto deploy * npm audit fix * npm audit fix
* [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
* [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
Replace generic proxy API detection with Firefox-specific runtime.getBrowserInfo method for more reliable browser identification.
Summary of ChangesHello @SYM01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the proxy management system by introducing per-tab proxy detection and status display, particularly for auto-switch configurations. It refines the browser API abstraction layer to ensure broader compatibility and implements a robust mechanism for dynamically selecting proxy profiles based on URL patterns. These changes aim to provide users with more transparency and control over their proxy settings, alongside general maintenance and dependency updates. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature for "auto-switch" mode, allowing users to see which proxy profile is active for the current tab. It also includes numerous fixes and improvements, such as updating dependencies, improving Firefox compatibility, and fixing issues with storing reactive objects.
The implementation is well-structured, with new services for statistics collection (WebRequestStatsService) and PAC script simulation (pacSimulator). The adapter pattern is effectively used to handle browser differences, and the addition of comprehensive tests for the new logic is commendable.
I've found a few issues that need attention:
- A critical issue in the Firefox adapter's
onWebRequestAuthRequiredimplementation that will likely break proxy authentication. - A high-severity bug in PAC script generation that could lead to incorrect proxy settings.
- A medium-severity issue regarding the use of
setTimeoutas a workaround, which could be fragile.
Overall, this is a great set of changes that adds valuable functionality. Addressing the identified issues will make the implementation more robust and reliable.
| onWebRequestAuthRequired( | ||
| callback: ( | ||
| details: WebAuthenticationChallengeDetails, | ||
| callback?: (response: BlockingResponse) => void | ||
| ) => void | ||
| asyncCallback?: (response: BlockingResponse) => void | ||
| ) => BlockingResponse | undefined | ||
| ): void { | ||
| browser.webRequest.onAuthRequired.addListener( | ||
| callback, | ||
| callback as any, | ||
| { urls: ["<all_urls>"] }, | ||
| ["asyncBlocking"] | ||
| ); | ||
| } |
There was a problem hiding this comment.
The implementation of onWebRequestAuthRequired for Firefox appears to be incorrect. The use of callback as any hides a type mismatch between the expected listener for browser.webRequest.onAuthRequired and the provided callback.
Firefox's onAuthRequired listener, when using asyncBlocking, expects a function that returns a Promise<BlockingResponse>. However, the provided callback (ProxyAuthProvider.onAuthRequired) is designed for Chrome's API and uses an asyncCallback argument instead of returning a promise. This will likely cause proxy authentication to fail on Firefox as the promise is never resolved.
To fix this, you should wrap the callback in a new function that returns a promise, and resolve it inside the asyncCallback.
onWebRequestAuthRequired(
callback: (
details: WebAuthenticationChallengeDetails,
asyncCallback?: (response: BlockingResponse) => void
) => BlockingResponse | undefined
): void {
const listener = (
details: WebAuthenticationChallenge-details
): Promise<BlockingResponse> => {
return new Promise((resolve) => {
// The callback can either return a response synchronously or call asyncCallback.
const syncResponse = callback(details, (asyncResponse) => {
resolve(asyncResponse);
});
// If it returned a response synchronously, resolve with it.
if (syncResponse) {
resolve(syncResponse);
}
});
};
browser.webRequest.onAuthRequired.addListener(
listener,
{ urls: ["<all_urls>"] },
["asyncBlocking"]
);
}| if (["http", "https"].includes(scheme)) { | ||
| return PACScriptHelper.newSimpleLiteral( | ||
| `${cfg.scheme == "http" ? "PROXY" : "HTTPS"} ${host}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
There's a subtle bug in this if condition. You are checking cfg.scheme == "http", but cfg.scheme can be undefined. You've correctly defined a scheme constant on line 43 to handle this case by defaulting to "http". You should use that scheme constant in this condition as well.
When cfg.scheme is undefined, scheme becomes "http", but cfg.scheme == "http" evaluates to false, causing the function to incorrectly generate an HTTPS proxy string instead of a PROXY string.
| if (["http", "https"].includes(scheme)) { | |
| return PACScriptHelper.newSimpleLiteral( | |
| `${cfg.scheme == "http" ? "PROXY" : "HTTPS"} ${host}` | |
| ); | |
| } | |
| if (["http", "https"].includes(scheme)) { | |
| return PACScriptHelper.newSimpleLiteral( | |
| `${scheme == "http" ? "PROXY" : "HTTPS"} ${host}` | |
| ); | |
| } |
|
|
||
| if (ret.profile && ret.isConfident) { | ||
| const profile = ret.profile.profile; | ||
| setTimeout(() => setIndicator(profile, details.tabId), 50); // Sometimes the indicator doesn't update properly in Chrome, so we need to wait a bit. |
There was a problem hiding this comment.
Using setTimeout with an arbitrary delay to work around an issue is often a sign of a race condition and can be unreliable. The comment indicates this is a workaround for an indicator update issue in Chrome.
While this might work in most cases, it's fragile. It would be more robust to investigate the root cause of the update issue. Is there another browser event you could listen for that would guarantee the UI is ready for an update? For example, chrome.tabs.onUpdated with a status of complete might be a more reliable signal. Relying on a fixed delay can lead to inconsistent behavior on different machines or under heavy load.
No description provided.